Skip to content

Fix MSVC warning for potential mod by 0 (C4724) #106634

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nukethebees
Copy link

With dev_mode=yes, MSVC warns that some modulo expressions may be x % 0 and refuses to compile.

By assigning the size variables to local variables, the compiler knows size > 0 if the loop bodies are entered. Without the local variable, it's technically possible for size() to become zero after entering the body.

https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4724?view=msvc-170

@nukethebees nukethebees requested a review from a team as a code owner May 20, 2025 12:48
@akien-mga akien-mga added this to the 4.5 milestone May 20, 2025
@akien-mga akien-mga changed the title Fixed MSVC warning for potential mod by 0 (C4724) Fix MSVC warning for potential mod by 0 (C4724) May 20, 2025
@nukethebees
Copy link
Author

nukethebees commented May 20, 2025

I'm adding static casts to int to fix the failing builds. Will commit soon.

@akien-mga
Copy link
Member

Vector::size returns int64_t, I don't think you need an explicit cast here. We usually just use sizes like this:

const int size = vector.size();

@nukethebees
Copy link
Author

Vector::size returns int64_t, I don't think you need an explicit cast here. We usually just use sizes like this:

const int size = vector.size();

Sorry, I didn't read the return type closely enough. I'm used to container size types always being unsigned e.g. std::size_t.

Either way, the original usage of uint64_t was wrong and the current cast makes the implicit conversion explicit. I'll fix the incorrect commit message.

@akien-mga
Copy link
Member

We don't usually use explicit static casts in such cases. Our consensus is that it makes code harder to read and doesn't add much value.

@nukethebees
Copy link
Author

We don't usually use explicit static casts in such cases. Our consensus is that it makes code harder to read and doesn't add much value.

Should I remove the casts or just leave them?

@akien-mga
Copy link
Member

I would just use const int points_size = points.size(); if it works.

@akien-mga
Copy link
Member

Looks good! Could you squash the commits? See PR workflow for instructions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants